Though both openzeppelin-solidity and @openzeppelin/contracts npm repositories are still being updated, @openzeppelin/contracts seems too be the preferred repo now (it's the one mentioned on their Github page).

I had no idea about the new one. Will update.

Not related to this PR but this should say "contract" not "module"

Is super. required here? You didn't use it in TestERC721.mint()

Probably inconsequential but maybe worth notifying the clients that whereas before Received could not have been emitted with both msg.value == 0 and msg.data == 0, it is now possible.

I couldn't work out what's right for this event. Here it is emitted for 0 values while in the Proxy it was not.

Yes the previous implementation was confusing. I think it would have made more sense to check if (msg.data.length == 0) { emit ... } in the Proxy instead of if (msg.data.length == 0 && msg.value > 0) { emit ... }. The former is equivalent to your new implementation, which I think is good.

I think we can remove this event emission and leave receive() empty. If I'm not mistaken receive() in BaseWallet will only ever be called in our tests.

I've made Receive only emit on non-zero values 12c79ec

You probably tried the same things but I would have expected to be able to make TransferManager.addModule() call OnlyOwnerModule.addModule(_wallet, _module); instead of duplicating OnlyOwnerModule's code. Or alternatively, make OnlyOwnerModule be the last contract inherited by TransferManager (instead of LimitManager) and use super.addModule(_wallet, _module); in TransferManager.addModule(). But none of these solutions seem to work for me. Is that a Solidity bug or am I missing something?

Also, if we're going to duplicate OnlyOwnerModule's code, let's maybe change "BM: module is not registered" to "TM: module is not registered".

It's because addModule is external, see Chris's explanation in this discussion ethereum/solidity#3881 (comment)
We have the option of removing addModule from the IModule interface, make it public and then use super. Atm it's not used off that interface so I could go down that route. What do you think?

I personally think it is important to keep the addModule method in the IModule interface as it will protect us from future mistake.

Guaranteeing that every module can add another module combined with the fact that a wallet needs at least one module ensures the wallet can never be in a blocked state (in the sense of not being upgradable).

Yet another way is to switch IModule from an interface to abstract contract which allows public functions with no implementation at the same time as mandating all functions to be implemented.

Yes that works for me.

We could keep IModule as-is but make addModule in BaseModule and OnlyOwnerModule be public instead of external.

That's also an option. I don't feel strongly for either of the two. I'll just go with your version as it causes less changes to current state. Let me know if that's okay

This function is not needed and can be removed entirely. It will never be reached from the Proxy (since calls to the Proxy with msg.data == 0 are not forwarded to the BaseWallet).

It's actually used in tests as we're not creating wallets that are Proxies but just new-ing up a BaseWallet instance. It's arguable whether this is correct and we shouldn't be testing with a proper Proxy-based wallet.

It helps fix the coverage problem though as I can remove the logic of receive which allows coverage to pass.

Removing that method won't break the tests as they currently stand. The fallback() of BaseWallet will be executed instead, so I believe we can remove receive() regardless of whether we decide to rewrite the tests to use Proxy or not

I does break the tests as fallback won't execute when there's no calldata received. That was the whole point of splitting the fallback function.

As per the doc:

A payable fallback function is also executed for plain Ether transfers, if no receive Ether function is present. [...]

https://solidity.readthedocs.io/en/v0.6.10/contracts.html#fallback-function

I also verified in Remix that the following contract is able to receive ether via empty calldata:

pragma solidity ^0.6.10;
contract FallbackTest {
    fallback() external payable {}
}

Okay it's not failing because plain ether is not accepted but because the call to fallback costs more than 21000 gas. https://app.circleci.com/pipelines/github/argentlabs/argent-contracts/707/workflows/26010b94-04c4-4a0c-ad62-c9cbe0e39afe/jobs/897
I can't think of any way to completely remove the receive function off BaseWallet really but feel free to push any changes you think are possible.

I see. I don't love the idea of having a receive() function in BaseWallet that is only there for tests. On the other hand having to use a Proxy + BaseWallet in all our tests feels a bit heavy. Maybe using a BaseWalletTest in our tests, that inherits from BaseWallet and only adds an empty receive() to BaseWallet could be a middle ground? If not, let's just keep the empty receive().

Thinking about this more, testing with a Proxy + BaseWallet is probably the right move. We could maybe add a createWallet() method to test-manager.js that deploys both contracts + wraps the proxy with the BaseWallet abi.